Scheduler - Simplify Form scheduler proxy to config-based initialization#33422
Scheduler - Simplify Form scheduler proxy to config-based initialization#33422aleksei-semikozov wants to merge 5 commits intoDevExpress:26_1from
Conversation
aleksei-semikozov
changed the title
There was a problem hiding this comment.
Pull request overview
This PR refactors Scheduler appointment form integration by replacing the AppointmentForm “scheduler proxy” object with an explicit, typed configuration object.
Changes:
- Introduced
AppointmentFormConfig/RecurrenceFormConfigand switched AppointmentForm/RecurrenceForm to consume plain config instead of a Scheduler proxy. - Updated Scheduler option-change handling to recreate the appointment form/popup via
createAppointmentPopupForm()when relevant options change. - Updated internal Scheduler appointment popup test mocks to construct AppointmentForm using the new config shape.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/devextreme/js/__internal/scheduler/m_scheduler.ts | Passes a typed AppointmentFormConfig into AppointmentForm and recreates popup/form on key option changes. |
| packages/devextreme/js/__internal/scheduler/appointment_popup/m_recurrence_form.ts | Replaces Scheduler dependency with a minimal RecurrenceFormConfig for first-day-of-week and component creation. |
| packages/devextreme/js/__internal/scheduler/appointment_popup/m_form.ts | Replaces Scheduler proxy usage with AppointmentFormConfig and threads config through recurrence form creation. |
| packages/devextreme/js/__internal/scheduler/tests/mock/create_appointment_popup.ts | Updates mocks to build AppointmentForm using the new config object. |
| editing: this.editing, | ||
| resourceManager: this.resourceManager, | ||
| firstDayOfWeek: this.getFirstDayOfWeek(), | ||
| startDayHour: this.option('startDayHour') ?? 0, |
There was a problem hiding this comment.
startDayHour in AppointmentFormConfig is taken from this.option('startDayHour'), while other view-dependent values (e.g. firstDayOfWeek) are resolved via view options. If startDayHour is overridden per-view (views[].startDayHour), the appointment form will use the global value instead of the effective current-view value, which can lead to incorrect start/end times when toggling All Day. Consider using this.getViewOption('startDayHour') (with the same nullish fallback) to match current-view behavior (see scheduler_options_base_widget.ts:getViewOption).
| startDayHour: this.option('startDayHour') ?? 0, | |
| startDayHour: this.getViewOption('startDayHour') ?? 0, |
| const element = $('<div>'); | ||
| const editingConfig = this.scheduler.getEditingConfig(); | ||
| const { | ||
| items: formItems, onContentReady, onInitialized, ...customFormOptions |
There was a problem hiding this comment.
formItems is destructured from this.getEditingForm() but never used. If the intent is just to omit items from customFormOptions, consider renaming it to an intentionally-unused name (e.g. _formItems) or adjusting the destructuring to avoid introducing an unused local (helps keep eslint/TS no-unused-vars clean).
| items: formItems, onContentReady, onInitialized, ...customFormOptions | |
| items: _formItems, onContentReady, onInitialized, ...customFormOptions |
| this.setRemoteFilterIfNeeded(); | ||
|
|
||
| this.postponeDataSourceLoading(); | ||
| this.createAppointmentPopupForm(); |
There was a problem hiding this comment.
In the startDayHour/endDayHour branch, createAppointmentPopupForm() is called for both options. Since the form config snapshots startDayHour but does not directly depend on endDayHour (end-day changes are already reflected via getCalculatedEndDate calling into the workspace), consider calling createAppointmentPopupForm() only when name === 'startDayHour' to avoid unnecessary popup/form re-creation on endDayHour changes.
| this.createAppointmentPopupForm(); | |
| if (name === 'startDayHour') { | |
| this.createAppointmentPopupForm(); | |
| } |
sjbur
left a comment
sjbur
left a comment
There was a problem hiding this comment.
Please take a look at the following demo: https://codepen.io/sjbur/pen/VYmwPZb?editors=0011
This is how the scheduler form behaved before this refactoring with async resources:
- The form shows immediately the new resource type (it shows ownerId instead of old roomId, it when you click on roomId it shows loading state, because it is being loaded)
- The demo does not have any errors, but I see them with your code (m_scheduler.js:1277 Uncaught (in promise) TypeError: Cannot read properties of undefined (reading '_dispose')
How to fix behavior with resourses: add form recreation on resources change:
m_scheduler.ts:
case 'resources':
this.resourceManager?.dispose();
this.resourceManager = new ResourceManager(this.option('resources'));
this.updateAppointmentDataSource();
this.createAppointmentPopupForm();
|
Check following demo: https://codepen.io/sjbur/pen/VYmwPZb?editors=0011 Open the start date calendar: first day of the week display correctly (it must be Monday), with current code it display Sunday (wrong, because firstDayOfWeek is being calculated once) |
No description provided.